-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang-tidy] Add new check: readability-redundant-typename
#161574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Victor Chernyakin (localspook) ChangesCloses #158374. I did my best to cover all cases where the new rules apply, but it's of course possible I missed some. The actual paper that introduced this feature was P0634, but it gives so few examples, and as someone not deeply familiar with standardese, it was really hard to figure out in what cases the new rule applied. What saved me was finding P2150: this paper goes case-by-case, listing all the places Full diff: https://github.com/llvm/llvm-project/pull/161574.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
index 4b4c49d3b17d1..881672e36eb7d 100644
--- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
@@ -48,6 +48,7 @@ add_clang_library(clangTidyReadabilityModule STATIC
RedundantSmartptrGetCheck.cpp
RedundantStringCStrCheck.cpp
RedundantStringInitCheck.cpp
+ RedundantTypenameCheck.cpp
ReferenceToConstructedTemporaryCheck.cpp
SimplifyBooleanExprCheck.cpp
SimplifySubscriptExprCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
index d01882dfc9daa..6ff64209a2b0e 100644
--- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -51,6 +51,7 @@
#include "RedundantSmartptrGetCheck.h"
#include "RedundantStringCStrCheck.h"
#include "RedundantStringInitCheck.h"
+#include "RedundantTypenameCheck.h"
#include "ReferenceToConstructedTemporaryCheck.h"
#include "SimplifyBooleanExprCheck.h"
#include "SimplifySubscriptExprCheck.h"
@@ -140,6 +141,8 @@ class ReadabilityModule : public ClangTidyModule {
"readability-redundant-member-init");
CheckFactories.registerCheck<RedundantPreprocessorCheck>(
"readability-redundant-preprocessor");
+ CheckFactories.registerCheck<RedundantTypenameCheck>(
+ "readability-redundant-typename");
CheckFactories.registerCheck<ReferenceToConstructedTemporaryCheck>(
"readability-reference-to-constructed-temporary");
CheckFactories.registerCheck<SimplifySubscriptExprCheck>(
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
new file mode 100644
index 0000000000000..2861cbb19e534
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
@@ -0,0 +1,71 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "RedundantTypenameCheck.h"
+#include "clang/AST/TypeLoc.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Sema/DeclSpec.h"
+
+using namespace clang::ast_matchers;
+using namespace clang::ast_matchers::internal;
+
+namespace clang::tidy::readability {
+
+void RedundantTypenameCheck::registerMatchers(MatchFinder *Finder) {
+ // NOLINTNEXTLINE(readability-identifier-naming)
+ const VariadicDynCastAllOfMatcher<TypeLoc, TypedefTypeLoc> typedefTypeLoc;
+ Finder->addMatcher(typedefTypeLoc().bind("typeloc"), this);
+
+ if (!getLangOpts().CPlusPlus20)
+ return;
+
+ // NOLINTBEGIN(readability-identifier-naming)
+ const VariadicDynCastAllOfMatcher<Stmt, CXXNamedCastExpr> cxxNamedCastExpr;
+ const auto inImplicitTypenameContext = [&] {
+ return anyOf(hasParent(typedefNameDecl()),
+ hasParent(templateTypeParmDecl()),
+ hasParent(nonTypeTemplateParmDecl()),
+ hasParent(cxxNamedCastExpr()), hasParent(cxxNewExpr()),
+ hasParent(friendDecl()), hasParent(fieldDecl()),
+ hasParent(parmVarDecl(hasParent(expr(requiresExpr())))),
+ hasParent(parmVarDecl(hasParent(typeLoc(hasParent(namedDecl(
+ anyOf(cxxMethodDecl(), hasParent(friendDecl()),
+ functionDecl(has(nestedNameSpecifier()))))))))),
+ // Match return types.
+ hasParent(functionDecl(unless(cxxConversionDecl()))));
+ };
+ // NOLINTEND(readability-identifier-naming)
+ Finder->addMatcher(typeLoc(inImplicitTypenameContext()).bind("typeloc"),
+ this);
+}
+
+void RedundantTypenameCheck::check(const MatchFinder::MatchResult &Result) {
+ const SourceLocation TypenameKeywordLoc = [&] {
+ if (const auto *TTL = Result.Nodes.getNodeAs<TypedefTypeLoc>("typeloc"))
+ return TTL->getElaboratedKeywordLoc();
+
+ TypeLoc InnermostTypeLoc = *Result.Nodes.getNodeAs<TypeLoc>("typeloc");
+ while (const TypeLoc Next = InnermostTypeLoc.getNextTypeLoc())
+ InnermostTypeLoc = Next;
+
+ if (const auto DNTL = InnermostTypeLoc.getAs<DependentNameTypeLoc>())
+ return DNTL.getElaboratedKeywordLoc();
+
+ return SourceLocation();
+ }();
+
+ if (!TypenameKeywordLoc.isValid())
+ return;
+
+ diag(TypenameKeywordLoc, "redundant 'typename'")
+ << FixItHint::CreateRemoval(TypenameKeywordLoc);
+}
+
+} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.h b/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.h
new file mode 100644
index 0000000000000..2df5b38dcef0b
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.h
@@ -0,0 +1,37 @@
+
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTTYPENAMECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTTYPENAMECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::readability {
+
+/// Finds unnecessary uses of the `typename` keyword.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability/redundant-typename.html
+class RedundantTypenameCheck : public ClangTidyCheck {
+public:
+ RedundantTypenameCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.CPlusPlus;
+ }
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ std::optional<TraversalKind> getCheckTraversalKind() const override {
+ return TK_IgnoreUnlessSpelledInSource;
+ }
+};
+
+} // namespace clang::tidy::readability
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTTYPENAMECHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3f403c42a168a..8f87864eff036 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -199,6 +199,11 @@ New checks
Finds virtual function overrides with different visibility than the function
in the base class.
+- New :doc:`readability-redundant-typename
+ <clang-tidy/checks/readability/redundant-typename>` check.
+
+ Finds unnecessary uses of the ``typename`` keyword.
+
New check aliases
^^^^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index e06849c419389..2373edc3a2e96 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -408,6 +408,7 @@ Clang-Tidy Checks
:doc:`readability-redundant-smartptr-get <readability/redundant-smartptr-get>`, "Yes"
:doc:`readability-redundant-string-cstr <readability/redundant-string-cstr>`, "Yes"
:doc:`readability-redundant-string-init <readability/redundant-string-init>`, "Yes"
+ :doc:`readability-redundant-typename <readability/redundant-typename>`, "Yes"
:doc:`readability-reference-to-constructed-temporary <readability/reference-to-constructed-temporary>`,
:doc:`readability-simplify-boolean-expr <readability/simplify-boolean-expr>`, "Yes"
:doc:`readability-simplify-subscript-expr <readability/simplify-subscript-expr>`, "Yes"
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/redundant-typename.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/redundant-typename.rst
new file mode 100644
index 0000000000000..1c4040fbd12df
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/redundant-typename.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - readability-redundant-typename
+
+readability-redundant-typename
+==============================
+
+Finds unnecessary uses of the ``typename`` keyword.
+
+``typename`` is unnecessary in two cases. First, before non-dependent names:
+
+.. code-block:: c++
+
+ /* typename */ std::vector<int>::size_type size;
+
+And second, since C++20, before dependent names that appear in a context
+where only a type is allowed (the following example shows just a few of them):
+
+.. code-block:: c++
+
+ template <typename T>
+ using trait = /* typename */ T::type;
+
+ template <typename T>
+ struct S {
+ /* typename */ T::type variable;
+ /* typename */ T::type function(/* typename */ T::type);
+ };
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename.cpp
new file mode 100644
index 0000000000000..3e77213564f89
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename.cpp
@@ -0,0 +1,191 @@
+// RUN: %check_clang_tidy -std=c++11,c++14,c++17 %s readability-redundant-typename %t \
+// RUN: -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy -std=c++20-or-later -check-suffixes=,20 %s readability-redundant-typename %t \
+// RUN: -- -- -fno-delayed-template-parsing
+
+struct NotDependent {
+ using R = int;
+};
+
+auto f(typename NotDependent::R)
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES: auto f(NotDependent::R)
+ -> typename NotDependent::R
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES: -> NotDependent::R
+{
+ return typename NotDependent::R();
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: redundant 'typename' [readability-redundant-typename]
+ // return NotDependent::R();
+}
+
+template <
+ typename T,
+ typename T::R V,
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:3: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: T::R V,
+ typename U = typename T::R
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:16: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: typename U = T::R
+>
+auto f() -> typename T::R
+// CHECK-MESSAGES-20: :[[@LINE-1]]:13: warning: redundant 'typename' [readability-redundant-typename]
+// CHECK-FIXES-20: auto f() -> T::R
+{
+ static_cast<typename T::R>(0);
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:15: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: static_cast<T::R>(0);
+
+ dynamic_cast<typename T::R>(0);
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:16: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: dynamic_cast<T::R>(0);
+
+ reinterpret_cast<typename T::R>(0);
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:20: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: reinterpret_cast<T::R>(0);
+
+ const_cast<typename T::R>(0);
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:14: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: const_cast<T::R>(0);
+
+ static_cast<typename T::R&>(0);
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:15: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: static_cast<T::R&>(0);
+
+ dynamic_cast<typename T::R const volatile &&>(0);
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:16: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: dynamic_cast<T::R const volatile &&>(0);
+
+ reinterpret_cast<const typename T::template M<42>::R *>(0);
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:26: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: reinterpret_cast<const T::template M<42>::R *>(0);
+
+ const_cast<const typename T::R *const[100]>(0);
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:20: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: const_cast<const T::R *const[100]>(0);
+
+ (typename T::R)(0);
+
+ alignof(typename T::R);
+
+ new typename T::R();
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:7: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: new T::R();
+
+ // CHECK-MESSAGES-20: :[[@LINE+2]]:15: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: static_cast<decltype([] {
+ static_cast<typename decltype([] {
+ return typename T::R(); // Inner typename must stay.
+ })::R>(0);
+
+ auto localFunctionDeclaration() -> typename T::R;
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:38: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: auto localFunctionDeclaration() -> T::R;
+
+ void (*PointerToFunction)(typename T::R);
+ void anotherLocalFunctionDeclaration(typename T::R);
+
+ typename T::R DependentVar;
+ typename NotDependent::R NotDependentVar;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES: NotDependent::R NotDependentVar;
+
+ return typename T::R();
+}
+
+template <typename T>
+using trait = const typename T::R ****;
+// CHECK-MESSAGES-20: :[[@LINE-1]]:21: warning: redundant 'typename' [readability-redundant-typename]
+// CHECK-FIXES-20: using trait = const T::R ****;
+
+template <typename T>
+trait<typename T::R> m();
+
+#if __cplusplus >= 202002L
+
+template <typename T>
+concept c = requires(typename T::R) {
+// CHECK-MESSAGES-20: :[[@LINE-1]]:22: warning: redundant 'typename' [readability-redundant-typename]
+// CHECK-FIXES-20: concept c = requires(T::R) {
+ typename T::R;
+};
+
+template <typename T>
+requires c<typename T::R>
+void b();
+
+#endif // __cplusplus >= 202002L
+
+template <typename T, typename>
+struct PartiallySpecializedType {};
+
+template <typename T>
+struct PartiallySpecializedType<T, typename T::R> {};
+
+template <typename T>
+auto v = typename T::type();
+
+template <typename T>
+typename T::R f();
+// CHECK-MESSAGES-20: :[[@LINE-1]]:1: warning: redundant 'typename' [readability-redundant-typename]
+// CHECK-FIXES-20: T::R f();
+
+template <typename T>
+void n(typename T::R);
+
+namespace ns {
+
+template <typename T>
+void f(typename T::R1, typename T::R2);
+
+} // namespace ns
+
+template <typename T>
+void ns::f(
+ typename T::R1,
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:3: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: T::R1,
+ typename T::R2
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:3: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: T::R2
+);
+
+template <typename T>
+class A {
+public:
+ friend typename T::R;
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:10: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: friend T::R;
+
+ typedef typename T::R a;
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:11: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: typedef T::R a;
+
+ const typename T::R typedef b;
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:9: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: const T::R typedef b;
+
+ typename T::R v;
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:3: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: T::R v;
+
+ typename T::R
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:3: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: T::R
+ g(typename T::R) {}
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:5: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: g(T::R) {}
+
+ void h(typename T::R = typename T::R()) {}
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:10: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: void h(T::R = typename T::R()) {}
+
+ friend void k(typename T::R) {}
+ // CHECK-MESSAGES-20: :[[@LINE-1]]:17: warning: redundant 'typename' [readability-redundant-typename]
+ // CHECK-FIXES-20: friend void k(T::R) {}
+
+ enum E1 : typename T::R {};
+ enum class E2 : typename T::R {};
+ operator typename T::R();
+ void m() { this->operator typename T::R(); }
+};
|
clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/readability/redundant-typename.rst
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
Outdated
Show resolved
Hide resolved
|
||
// NOLINTBEGIN(readability-identifier-naming) | ||
const VariadicDynCastAllOfMatcher<Stmt, CXXNamedCastExpr> cxxNamedCastExpr; | ||
const auto inImplicitTypenameContext = anyOf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have hasParent
in each entry of anyOf
, could we write instead: hasParent(anyOf(typedefNameDecl(), templateTypeParmDecl(), ...))
to lower number of repetitive hasParent
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't work as well as we might like, because some of those inner matchers match on expressions, and others on declarations, so you can't pass them all to one anyOf()
. It's still possible to reduce duplication, but it needs to look like:
anyOf(
hasParent(decl(anyOf(...))),
hasParent(expr(anyOf(...)))
)
I pushed a commit that does that. Honestly don't know which version I prefer, so I'll leave it to others to decide if it's an improvement.
clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.h
Outdated
Show resolved
Hide resolved
Co-authored-by: EugeneZelenko <[email protected]>
8e26231
to
8f33b21
Compare
Can we handle pack extension? template <typename... Ts> struct S {
void f(typename Ts::R...);
}; |
Ooh, that's a case I didn't consider. Looks like the existing logic handles them correctly though. |
Successfully ran the check over stdexec (well, successfully after fixing the two bugs it found) |
clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename-cxx98.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/readability/redundant-typename.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, It would be great if you run this check on libc++/boost (in c++20 mode) which must have a lot of templates to see if there are any more FP.
Closes #158374.
I did my best to cover all cases where the new rules apply, but it's of course possible I missed some.
The paper that introduced this feature is P0634. That paper contains standardese but few examples, and as someone not fluent in standardese, it was really hard to figure out in what cases the new rules applied. I was saved when I found P2150: this paper goes case-by-case, listing all the places
typename
is and isn't needed, and provides rationales for each one! The clang tests for this feature also helped.